Skip to content

Conversation

@vetinari
Copy link
Contributor

@vetinari vetinari commented May 5, 2017

conn.go Outdated
if timeout > 0 {
l.timeoutMutex.Lock()
l.requestTimeout = timeout
l.timeoutMutex.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be simpler to make this an atomic value and do lockless load/store. We already have a shim for < go1.3 using locks

@vetinari
Copy link
Contributor Author

vetinari commented May 8, 2017

using the atomicValue struct triggered the race during travis build...

@liggitt
Copy link
Contributor

liggitt commented May 8, 2017

using the atomicValue struct triggered the race during travis build...

that's concerning... looks like the 1.3 version needs to hold a pointer to the lock

@vetinari
Copy link
Contributor Author

vetinari commented May 8, 2017

1.3 was ok, it hit 1.5: https://travis-ci.org/go-ldap/ldap/builds/229842330, the 1.3 failure was a network fetch error while setting up: https://travis-ci.org/go-ldap/ldap/builds/229846728

@liggitt
Copy link
Contributor

liggitt commented May 9, 2017

hmm, that's still concerning, but the atomic load/store int64 is nice and clean and sidesteps that issue. go ahead and squash and remove the unused mutex, then LGTM

@vetinari
Copy link
Contributor Author

hmm... https://travis-ci.org/go-ldap/ldap/jobs/230628746 ... what does the race detector find? concurrent r/w access to the *Conn struct's members?

@liggitt liggitt mentioned this pull request Aug 24, 2017
This was referenced Sep 23, 2017
@johnweldon
Copy link
Member

@vetinari I think this is handled by the recent merge #134

@vetinari
Copy link
Contributor Author

fixed by #134

@vetinari vetinari closed this Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants